WIP feat(DRIVERS-3344): add support for deprioritized servers to all topologies #4821
Conversation
| {}, | ||
| { serverSelectionTimeoutMS: 10 } | ||
| ); | ||
| const topology = topologyWithPlaceholderClient('someserver:27019', { |
There was a problem hiding this comment.
NB: this change is not necessary (along with the change in tools/utils.ts) but without this change, we don't actually use serverSelectionTimeoutMS: 10 in the test, and the test takes 30s instead of 10ms.
The test works just fine either way.
baileympearson
force-pushed
the
DRIVERS-3344
branch
from
e46b986 to
67081d4
Compare
| ) => ServerDescription[]; | ||
|
|
||
| /** @internal */ | ||
| export class DeprioritizedServers { |
There was a problem hiding this comment.
This could also have either been a type alias to Set<string> or a subclass of Set<string> but I liked the dedicated class approach because:
- it is explicit and clear
- it doesn't expose extra APIs (i.e., no extra unneeded set methods)
- it is more convenient to interact with it externally using only ServerDescriptions, and it becomes an implementation detail that it only contains strings.
| const primary: ServerDescription = Array.from(topologyDescription.servers.values()).filter( | ||
| primaryFilter | ||
| )[0]; | ||
|
|
There was a problem hiding this comment.
There are a chunk of "clean ups" that I put in this PR. I've separated them into a single commit to make reviewing easier, but basically they boil down to just replacing Array.reduce calls that perform as filters with filter. You can view those changes by just reviewing the first commit of this PR.
| } | ||
|
|
||
| return true; | ||
| return Object.entries(tagSet).every( |
There was a problem hiding this comment.
misc cleanup. can revert.
| !deprioritized.has(server); | ||
| } | ||
|
|
||
| function secondarySelector( |
There was a problem hiding this comment.
The rest of the changes in this file are pretty in depth, but they helped me when I was writing this code and I think they'll help reviewers and devs going forward.
I migrated to switch statements for clarity and exhaustiveness checking.
| topologyDescription: TopologyDescription, | ||
| servers: ServerDescription[], | ||
| deprioritized?: ServerDescription[] | ||
| deprioritized: DeprioritizedServers |
There was a problem hiding this comment.
Making deprioritized servers is an opinionated choice that resulted in a ton of small test changes. I prefer this approach because forcing callers to provide this prevents scenarios where callers forget to provide DeprioritizedServers when they should.
Happy to reconsider though.
Description
Summary of Changes
Notes for Reviewers
What is the motivation for this change?
Release Highlight
Release notes highlight
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript